Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Review feedback addressed in c3d1e6fd:

Gemini MEDIUM #98 — Adapters nil-vs-empty must be distinguishable on disk
The non-pointer Adapter fields collapsed "excluded by --adapter filter" and "included with no scopes" into the same on-disk shape. Switched to *Adapter pointers + omitempty. Now: nil → excluded (key absent), non-nil empty &Adapter{} → included with no scopes ("dynamodb":{}), non-nil populated → normal. New TestAdaptersStruct_NilVsEmptyDistinguishedOnDisk covers it.

Codex P1 #179 — Validate base64 at parse time
KeymapReader.Next previously deferred base64 validation to rec.Original(). A malformed dump would parse fine and only fail on a much later read. Now the base64 is decoded inside Next() so corruption surfaces on first ingest. New TestKeymapReader_RejectsMalformedBase64AtParseTime.

Codex P2 #194 — Reject trailing bytes after MANIFEST JSON
ReadManifest now uses dec.More() after the single decode and rejects with ErrInvalidManifest if anything decodable remains. Catches concatenation bugs, partial writes, and double-write corruption. New TestReadManifest_RejectsTrailingBytes and TestReadManifest_RejectsTrailingNonWhitespace.

Gemini MEDIUM #160 — Test name fix
TestKeymapReader_AcceptsBlankLinesByPolicy → TestKeymapReader_RejectsBlankLines. The test was already asserting rejection; only the name lied.

@claude review

Claude finished @bootjp's task in 2m 38s —— View job

Review of c3d1e6fd — KEYMAP/MANIFEST follow-up fixes

Tasks:

• Read trigger comment and understand scope
• Read changed files (keymap.go, keymap_test.go, manifest.go, manifest_test.go)
• Diff against base branch to isolate the c3d1e6fd changes
• Review all four fixes claimed in the trigger comment
• Post full review

All four stated fixes are correctly implemented. One minor comment inaccuracy to address, everything else is clean.

Fix 1 — *Adapter pointers (Gemini #98) ✅

manifest.go:106-111 — The change from value-type Adapter fields to *Adapter + omitempty is correct and complete. nil → key absent (excluded), &Adapter{} → "dynamodb":{} (included, no scopes), populated *Adapter → normal. The field comment (manifest.go:91-105) explains the three states clearly.

TestAdaptersStruct_NilVsEmptyDistinguishedOnDisk (manifest_test.go:241-261) is well-targeted: it asserts "dynamodb":{} is present and that "s3", "redis", "sqs" are absent. ✓

One observation: TestManifest_Phase0RoundTrip (manifest_test.go:22-27) now populates all four adapters. There's no test for a round-trip where some adapters are nil (excluded) — this would complement the serialization-only test. Not a blocker, just worth noting.

Fix 2 — Parse-time base64 validation in Next() (Codex P1 #179) ✅

keymap.go:184-187 — The base64.RawURLEncoding.DecodeString call placed after the empty-field guard is exactly right. It decodes and discards the bytes in Next(), then callers who need the bytes call rec.Original() which decodes again. The double-decode is a minor overhead but correct — LoadKeymap uses it this way already via assertKeymapRecord. For the expected keymap sizes this is not a concern.

TestKeymapReader_RejectsMalformedBase64AtParseTime (keymap_test.go:207-219) uses "!!!" as the deliberately invalid base64. ✓

Fix 3 — Trailing-bytes rejection via dec.More() (Codex P2 #194) ✅ (with one comment bug)

manifest.go:216-219 — dec.More() correctly returns true if any non-whitespace content follows the decoded object. The two tests confirm:

• TestReadManifest_RejectsTrailingBytes — doubled manifest JSON → caught ✓
• TestReadManifest_RejectsTrailingNonWhitespace — "garbage" suffix → caught ✓

Minor bug: the comment is wrong in two places (manifest.go:209-215):

// MANIFEST.json is exactly one JSON object. Trailing bytes
// (a second object, junk, even whitespace-only padding) point at
// ...
// must surface here rather than be silently discarded. We use
// io.Discard rather than parsing because we only care that
// nothing-decodable is present; ...

1. "even whitespace-only padding" — dec.More() skips whitespace before peeking, so it returns false for a trailing newline or spaces. Trailing whitespace is not caught (and that's the correct behaviour — text editors add trailing newlines; this should not be an error). The comment incorrectly claims it is caught.
2. "We use io.Discard rather than parsing" — there is no io.Discard anywhere in this function. This sentence is stale (describes a design that was not used).

The correct comment would be something like: "dec.More() returns true for any non-whitespace decodable content; trailing whitespace/newlines are benign and pass."

Fix this →

Fix 4 — Test name rename (Gemini #160) ✅

TestKeymapReader_AcceptsBlankLinesByPolicy → TestKeymapReader_RejectsBlankLines. The body already asserted rejection; only the name was wrong. The updated name and comment (keymap_test.go:161-165) now agree with the assertion. ✓

Overall

The four fixes address the review findings accurately. The only actionable item is correcting the misleading two-part comment in ReadManifest around dec.More(). Everything else is correct.

Codex P1 #295 addressed in e4cd7d8e. Phase 1 manifests now require Live != nil AND Live.ReadTS != 0; both WriteManifest and ReadManifest reject a phase1 dump that omits its cluster-wide pin record. New tests: TestManifest_Phase1MustSetLive, TestManifest_Phase1RejectsZeroReadTS, TestManifest_Phase1WithLiveAndNonZeroReadTSIsValid.

@claude review

Claude finished @bootjp's task in 2m 29s —— View job

Review of e4cd7d8e — Phase 1 Live metadata validation (Codex P1 #295)

Tasks:

• Read trigger comment and understand scope
• Read manifest.go and manifest_test.go to review the fix
• Diff against base branch to isolate e4cd7d8e changes
• Review phase1 Live validation logic
• Review new tests
• Post full review

The core fix is correct and the tests cover all three branches. Two minor issues to address:

Fix — validatePhaseSpecific phase1 checks ✅

manifest.go:294-307 — The two additions are exactly right:

if m.Live == nil {
    return errors.Wrap(ErrInvalidManifest, "phase1 must set live")
}
if m.Live.ReadTS == 0 {
    return errors.Wrap(ErrInvalidManifest, "phase1 live.read_ts must be non-zero")
}

Ordering is safe: nil check before field dereference. Since validate() is shared by WriteManifest and ReadManifest, both paths enforce the constraint symmetrically — a hand-crafted phase1 JSON with live absent or read_ts: 0 will fail at ReadManifest too.

Tests ✅

All three new tests are well-targeted:

• TestManifest_Phase1MustSetLive — nil Live → ErrInvalidManifest ✓
• TestManifest_Phase1RejectsZeroReadTS — Live{ReadTS: 0} → ErrInvalidManifest ✓
• TestManifest_Phase1WithLiveAndNonZeroReadTSIsValid — round-trip with ReadTS: 12345, asserts got.Live.ReadTS == 12345 ✓

Minor: issue reference in comment violates CLAUDE.md

manifest.go:297-301:

// A phase1 dump's whole point is the cluster-wide read_ts
// pin recorded under Live. A manifest that omits Live cannot
// describe its consistency point and downstream restore /
// audit logic must not silently accept it as valid (Codex
// P1 #295).

CLAUDE.md says: "Do not reference the current task, fix, or callers… since those belong in the PR description and rot as the codebase evolves." Drop (Codex P1 #295) — the substantive explanation above it is sufficient.

Fix this →

Carry-over: stale comment from previous round still unaddressed

manifest.go:209-215 (added in c3d1e6fd, not fixed in e4cd7d8e):

// MANIFEST.json is exactly one JSON object. Trailing bytes
// (a second object, junk, even whitespace-only padding) point at
// concatenation bugs or partial-write corruption — both of which
// must surface here rather than be silently discarded. We use
// io.Discard rather than parsing because we only care that
// nothing-decodable is present; structural validation lives in
// validate().

Still has both issues flagged in the previous review round:

1. "even whitespace-only padding" — dec.More() skips whitespace so trailing newlines/spaces are not caught. That's the right behaviour, but the comment claims they are.
2. "We use io.Discard rather than parsing" — there is no io.Discard call in this function; this describes a design path that wasn't taken.

Suggested replacement:

// MANIFEST.json is exactly one JSON object. Any non-whitespace
// decodable content after it (a second object, junk) points at
// concatenation bugs or partial-write corruption. Trailing
// whitespace / newlines are benign and not flagged.

Fix this →

Summary

The Codex P1 #295 fix in e4cd7d8e is correct and complete. Two non-blocking cleanup items remain: drop the issue reference from the new comment, and fix the stale comment in ReadManifest that was flagged last round.

Codex P1 #205 (round 2) addressed in 6f231fe7. The earlier DisallowUnknownFields was too strict — it broke the documented same-major minor-evolution path. A newer producer that adds an optional field at the same format_version would turn older readers into hard read failures during mixed-version operation.

format_version IS the breaking-change signal: major bumps are gated by ErrUnsupportedFormatVersion, same-major minor evolution must silently tolerate optional new fields. Removed dec.DisallowUnknownFields(). The trailing-bytes guard (Codex P2 #194 from round 1) is preserved — it catches concatenation/corruption, which is a different concern.

Test TestReadManifest_RejectsUnknownFields replaced by TestReadManifest_AcceptsUnknownFieldsForSameMajorMinorEvolution.

@claude review

Claude finished @bootjp's task in 1m 50s —— View job

Review of 6f231fe7 — Unknown-fields tolerance for same-major evolution

Tasks:

• Read trigger comment and understand scope
• Read manifest.go and manifest_test.go
• Diff against base to isolate 6f231fe7 changes
• Review unknown-fields handling and test replacement
• Check carry-over items from previous rounds
• Post full review

Fix — Remove DisallowUnknownFields (Codex P1 #205 round 2) ✅

The removal is correct. The three-tier contract is now properly implemented:

• format_version > CurrentFormatVersion → hard refuse via ErrUnsupportedFormatVersion
• Same format_version + unknown fields → silently ignored (same-major minor evolution)
• dec.More() → still catches concatenation/corruption (distinct concern, correctly preserved)

TestReadManifest_AcceptsUnknownFieldsForSameMajorMinorEvolution (manifest_test.go:144-173) correctly asserts that a future_optional_field key passes through ReadManifest without error, and that the known fields still decode correctly. ✓

Issue reference in new comment (manifest.go:205-215) ⚠️

The new comment explaining the intentional omission of DisallowUnknownFields contains a ticket reference:

// We intentionally do NOT call DisallowUnknownFields here.
// The format-version contract (Codex P1, follow-up) is:

CLAUDE.md: "Do not reference the current task, fix, or callers… since those belong in the PR description and rot as the codebase evolves." Drop (Codex P1, follow-up) — the substantive explanation below it is self-sufficient.

Same issue in test comment (manifest_test.go:147-150) ⚠️

// Codex P1 #205 (round 2) caught the earlier
// DisallowUnknownFields strictness which broke the documented
// same-major compatibility model.

Same rule. The first two lines of the comment already convey the full rationale; the Codex reference adds nothing durable.

Carry-over: two items flagged in previous rounds still unaddressed

1. Stale trailing-bytes comment (manifest.go:219-225) — flagged in round 1 and round 2:

// MANIFEST.json is exactly one JSON object. Trailing bytes
// (a second object, junk, even whitespace-only padding) point at
// concatenation bugs or partial-write corruption — both of which
// must surface here rather than be silently discarded. We use
// io.Discard rather than parsing because we only care that
// nothing-decodable is present; structural validation lives in
// validate().

Both errors remain:

• "even whitespace-only padding" — dec.More() skips whitespace before peeking, so trailing newlines/spaces return false. They are not caught. That is the correct behaviour, but the comment claims they are.
• "We use io.Discard rather than parsing" — there is no io.Discard call anywhere in ReadManifest. This describes a design path that wasn't taken.

Fix this →

2. Issue reference in validatePhaseSpecific (manifest.go:307-311) — flagged in round 2:

// A phase1 dump's whole point is the cluster-wide read_ts
// pin recorded under Live. A manifest that omits Live cannot
// describe its consistency point and downstream restore /
// audit logic must not silently accept it as valid (Codex
// P1 #295).

(Codex P1 #295) is an issue reference that will rot. The three lines above it already convey the full rationale; drop the parenthetical.

Fix this →

Summary

The Codex P1 #205 fix in 6f231fe7 is correct. Three non-blocking comment cleanup items remain — all flagged in prior rounds — none of which affect correctness:

Codex P2 #146 (round 3) addressed in 73641330. ReadManifest previously accepted manifests where adapters and/or exclusions sections were omitted because they decoded into non-pointer struct fields and validate() only checked scalar policy fields. The empty zero-value sections passed silently, dropping provenance and masking truncation.

Manifest.Adapters / Manifest.Exclusions are now pointer types. validateRequiredFields rejects nil for either with ErrInvalidManifest. NewPhase0SnapshotManifest seeds both to non-nil zero values so producers satisfy the contract by default.

Tests: TestReadManifest_RejectsMissingAdapters, TestReadManifest_RejectsMissingExclusions.

@claude review

Claude finished @bootjp's task in 1m 47s —— View job

Review of 73641330 — Required adapters/exclusions sections (Codex P2 #146 round 3)

Tasks:

• Read trigger comment and understand scope
• Read manifest.go and manifest_test.go
• Diff against base to isolate 73641330 changes
• Review required-fields fix and new tests
• Check carry-over items from previous rounds
• Post full review

Fix — *Adapters / *Exclusions pointers + nil checks (Codex P2 #146 round 3) ✅

The approach is correct and complete. Changing Adapters/Exclusions from value to pointer types means an omitted JSON key decodes as nil, which validateRequiredFields now catches and rejects. The write path is also safe: WriteManifest calls validate() before encoding, so a nil-adapters manifest can never be written in the first place.

NewPhase0SnapshotManifest seeding &Adapters{} / &Exclusions{} is the right default — producers using the constructor satisfy the contract without extra ceremony, and the now-non-nil pointer is the correct on-disk shape for "present but empty". ✓

The two tests are well-targeted:

• TestReadManifest_RejectsMissingAdapters — adapters key absent → ErrInvalidManifest ✓
• TestReadManifest_RejectsMissingExclusions — exclusions key absent → ErrInvalidManifest ✓

TestManifest_Phase0RoundTrip updated from m.Adapters = Adapters{...} to m.Adapters = &Adapters{...}. ✓

Issue references in new comments — 3 new instances added this round ⚠️

The same pattern flagged in every prior round has appeared again, this time in three new locations:

manifest.go:144-148 (struct field comment):

// Adapters and Exclusions are pointer types so ReadManifest can
// distinguish "section omitted entirely" (a corrupted or
// truncated dump that should fail validation) from "section
// present but populated with default values" (legitimate
// scope-everything-excluded). Codex P2 #146 (round 3).

Drop Codex P2 #146 (round 3).

manifest.go:279-281 (validateRequiredFields):

// Adapters and Exclusions are required structural sections.
// A manifest that omits either is treated as truncated/corrupted
// (Codex P2 #146 round 3).

Drop (Codex P2 #146 round 3).

manifest_test.go:316-319 (TestReadManifest_RejectsMissingAdapters):

// Adapters section omitted from the JSON entirely — Codex P2
// #146 round 3. With Adapters as a pointer the omission decodes
// as nil; validation must surface ErrInvalidManifest rather than
// treat an empty zero-value section as valid.

Drop Codex P2 #146 round 3. from the first two lines (the rest is substantive and should stay).

Carry-over items — all still unaddressed after 4 rounds

The stale-comment fix has been written out in full in the round-1 and round-2 reviews. Repeating the suggested replacement for manifest.go:229-235:

// MANIFEST.json is exactly one JSON object. Any non-whitespace
// decodable content after it (a second object, junk) points at
// concatenation bugs or partial-write corruption. Trailing
// whitespace and newlines are benign and not flagged.

Fix all 7 issue references + stale comment in one pass →

Summary

The Codex P2 #146 round 3 fix in 73641330 is structurally correct. The only remaining work is comment cleanup — the fix logic and all new tests are sound. There are now 7 issue references across the two files, accumulated over 4 rounds, plus the stale io.Discard/whitespace comment that has been flagged since round 1. Addressing all of them in a single pass would close this PR cleanly.

@claude review

Round 5 fixes for Codex P2 findings on commit 7364133:

1. KeymapReader.Next (keymap.go) — now decodes into a presence-aware map[string]json.RawMessage first and rejects records missing any of {encoded, original, kind}, before unmarshaling into the typed struct. Previously a missing original field passed through because base64.DecodeString("") succeeds, silently rewriting the mapping to empty bytes. New regression tests TestKeymapReader_RejectsMissingOriginalField (rejection) and TestKeymapReader_AcceptsExplicitEmptyOriginal (explicit empty stays valid).

2. ReadManifest (manifest.go) — split into a relaxed format_version probe (only format_version uint32) followed by the strict struct decode. A future-major manifest that also changes the JSON type of a known field (e.g. phase string → int) now surfaces as ErrUnsupportedFormatVersion instead of ErrInvalidManifest, restoring the advertised version-branching contract. New regression test TestReadManifest_FutureMajorVersionTakesPrecedenceOverTypeMismatch.

Verified locally: go test -race ./internal/backup/... and golangci-lint run clean.

Claude finished @bootjp's task in 3m 3s —— View job

Review of Round 5 fixes (2cd58a9) — field-presence enforcement + version-first decode

Tasks:

• Read trigger comment and understand scope
• Read keymap.go and keymap_test.go — Fix 1 (presence-aware field checking)
• Read manifest.go and manifest_test.go — Fix 2 (version-first decode)
• Diff against base to isolate round 5 changes
• Check carry-over items from previous rounds
• Post full review

Both stated fixes are correct. Two new issue-reference violations in comments, and the stale ReadManifest comment (flagged since round 1) still has not been addressed.

Fix 1 — Two-phase decode in decodeKeymapLine ✅

keymap.go:192-216 — The two-phase approach (map probe then typed unmarshal) is the right tool for distinguishing "field absent" from "field present with empty string", and the implementation is clean. The ordered presence check ["encoded", "original", "kind"] returns a specific "missing field %q" error which is more informative than the previous silent pass-through.

TestKeymapReader_RejectsMissingOriginalField (keymap_test.go:227-243) — correctly targets the regression (absent original → error) and verifies the sticky-error invariant. ✓

TestKeymapReader_AcceptsExplicitEmptyOriginal (keymap_test.go:248-263) — confirms the complementary case: "original":"" is presence-present and passes through. ✓

One edge case not under test: "original": null (JSON null rather than absent). The map probe finds the key present; json.Unmarshal into KeymapRecord decodes null as "" for a string field; base64.DecodeString("") succeeds. So a null original is silently accepted as empty bytes — the same shape as an explicit "". Whether that is the intended contract is worth confirming. If null should be rejected, a json.RawMessage check after the map probe would catch it. Not a blocker unless the spec says otherwise, but worth documenting.

New issue references in decodeKeymapLine comment ⚠️

keymap.go:188,191:

//     a missing `original` would otherwise be silently rewritten to empty
//     bytes by base64.RawURLEncoding.DecodeString(""). Codex P2 round 5.
//  3. `original` (the base64) must be parseable at parse time so a
//     corrupted dump fails on first read rather than at later
//     Original() call. Codex P1 #179.

Drop Codex P2 round 5. and Codex P1 #179. — the substantive prose above each is sufficient.

New issue reference in keymap_test.go ⚠️

keymap_test.go:221:

// TestKeymapReader_RejectsMissingOriginalField exercises Codex P2 round 5:

Drop exercises Codex P2 round 5:.

Fix 2 — Version-first decode in ReadManifest ✅

manifest.go:213-274 — The two-phase structure (relaxed format_version-only probe via json.Unmarshal, then strict struct decode via json.NewDecoder) is exactly right. A future-major manifest that changes phase from string to int now surfaces as ErrUnsupportedFormatVersion at the probe step, before the strict decode ever runs. Using io.ReadAll once and then bytes.NewReader for the second pass avoids double-reading the io.Reader, which is correct.

TestReadManifest_FutureMajorVersionTakesPrecedenceOverTypeMismatch (manifest_test.go:140-160) — "format_version": 999 plus "phase": 42 tests exactly the stated scenario. The probe catches version 999 before the strict decode fails on integer phase. ✓

Note on extreme version break: If a future manifest changes format_version itself to a non-uint32 (e.g., "format_version": "2"), the relaxed probe's json.Unmarshal would fail and the error would surface as ErrInvalidManifest rather than ErrUnsupportedFormatVersion. This is an edge case far outside the stated contract and not worth handling, but the comment could acknowledge it.

New issue reference ⚠️

manifest.go:222:

// two-phase approach, a manifest produced by a newer major version
// that also changed the JSON type of a known field (e.g. `phase`
// switched from string to int) would surface as
// ErrInvalidManifest instead of ErrUnsupportedFormatVersion,
// breaking the documented version-branching contract for callers
// that key off errors.Is(err, ErrUnsupportedFormatVersion). See
// Codex P2, round 5.

Drop See Codex P2, round 5. — the three lines above it already document the why completely.

manifest_test.go:134:

// regression test for Codex P2 round 5: a newer-major manifest that also

Drop Codex P2 round 5:.

Carry-over: accumulated items unaddressed after 5 rounds

The corrected text for the stale manifest.go:260-266 comment (unchanged since round 1):

// MANIFEST.json is exactly one JSON object. Any non-whitespace
// decodable content after it (a second object, junk) points at
// concatenation bugs or partial-write corruption. Trailing
// whitespace and newlines are benign and not flagged.

Fix all accumulated comment issues in one pass →

Summary

The two Round 5 fixes in 2cd58a9 are correct. The field-presence check in decodeKeymapLine and the version-first probe in ReadManifest both address the stated issues accurately, and all new tests target the right regression paths. The only remaining work is comment cleanup: 4 new issue references added this round, plus 8 accumulated from prior rounds, plus the stale io.Discard/whitespace comment that has been flagged since round 1. A single cleanup pass across both files would close this out.

@claude review

Round 6 fix for Codex P1 on commit 2cd58a9:

The source store caps a single snapshot key at 1 MiB (store/mvcc_store.go maxSnapshotKeySize), but keymapBufSizeReader was also 1 MiB. KEYMAP records carry the key as base64url which expands ~4/3 — so a maximum-sized valid key produced a ~1.33 MiB record that bufio.Scanner refused with "token too long", and the dump could not be loaded back via LoadKeymap / KeymapReader. Bumped the scanner cap to 4 MiB (2× margin over the theoretical worst case).

Test: TestKeymapReader_AcceptsMaxSizedOriginal round-trips a 1 MiB original key through writer→reader. go test -race and golangci-lint clean. Fix merged forward into #713 / #714 / #716 / #718.

Claude finished @bootjp's task in 2m 11s —— View job

Review of Round 6 fix (0afb5e3) — KEYMAP scanner cap

Tasks:

• Read trigger comment and understand scope
• Read keymap.go and keymap_test.go to review the fix
• Diff against base branch to isolate round 6 changes
• Check carry-over items from previous rounds
• Post full review

The fix is correct. Two new code-quality issues added this round, and the accumulated carry-over items are still present.

Fix — keymapBufSizeReader 1 MiB → 4 MiB ✅

keymap.go:55 — The value change is exactly right. A 1 MiB key encodes to ~1.33 MiB of base64url plus ~80 bytes of JSON overhead, exceeding the old 1 MiB cap and causing bufio.Scanner: token too long. 4 MiB fits the worst-case record with ~3× headroom (the comment says "2× margin", which technically understates it since 4 MiB / 1.34 MiB ≈ 3.0×, but it is defensible as "at least 2×"). The constant comment explains the math at keymap.go:44-54.

TestKeymapReader_AcceptsMaxSizedOriginal (keymap_test.go:253-280) correctly round-trips a full 1 MiB original through writer→reader and verifies byte-for-byte equality. ✓

Issue: //nolint:mnd in test (keymap_test.go:258) ⚠️

original[i] = byte(i % 251) //nolint:mnd // arbitrary byte spread

CLAUDE.md: "Avoid //nolint — refactor instead." The fix is a named constant:

const testBytePrimeSpread = 251
...
original[i] = byte(i % testBytePrimeSpread)

Fix this →

New issue references added this round ⚠️

keymap.go:50-51:

// therefore not enough to cover a maximum-sized valid key — Codex P1
// round 6 (commit 2cd58a93). 4 MiB carries 2× margin...

Drop — Codex P1 round 6 (commit 2cd58a93).

keymap_test.go:245-246:

// TestKeymapReader_AcceptsMaxSizedOriginal is the regression for Codex
// P1 round 6: a record whose `original`...

Change is the regression for Codex P1 round 6: → verifies that.

Fix both →

Carry-over: accumulated items unaddressed after 6 rounds

The stale trailing-bytes comment (manifest.go:260-266) replacement (unchanged since round 1):

// MANIFEST.json is exactly one JSON object. Any non-whitespace
// decodable content after it (a second object, junk) points at
// concatenation bugs or partial-write corruption. Trailing
// whitespace and newlines are benign and not flagged.

Fix all accumulated comment issues in one pass →

Summary

The Round 6 buffer-cap fix in 0afb5e3 is correct — the size increase is well-reasoned and the regression test covers the exact failure mode. Three items to clean up: replace the //nolint:mnd with a named constant, drop the two new issue references, and address the 8 accumulated comment issues in manifest.go/manifest_test.go that have been flagged since rounds 1–5.

@claude review

Round 7 fixes for Codex round 7 on commit 0afb5e3 (commit 6b2dea00):

1. P1 — null KEYMAP fields. decodeKeymapLine was rejecting missing fields but accepting "original": null because Go's json.Unmarshal collapses null and "" for string targets, and base64.DecodeString("") succeeds. Added an explicit json.RawMessage compare against the literal null token; the same guard applies to encoded and kind for symmetry. Test: TestKeymapReader_RejectsExplicitNullField (3 sub-cases).

2. P2 — missing exclusion flags. validateRequiredFields only checked Exclusions != nil. Each bool field defaulted to false on absence, so a manifest dropping preserve_sqs_visibility (or any of the four) silently passed with altered exclusion semantics — losing the producer-side provenance the section is meant to capture. Added validateExclusionsFieldsPresent that walks the raw JSON and rejects manifests missing or null'ing any flag. Test: TestReadManifest_RejectsMissingExclusionFlag (5 sub-cases).

go test -race and golangci-lint clean. Fix merged forward into #713 / #714 / #716 / #718.

Claude finished @bootjp's task in 3m 17s —— View job

Review of Round 7 fixes (6b2dea00) — null KEYMAP fields + exclusion flag presence

Tasks:

• Read trigger comment and understand scope
• Read keymap.go and keymap_test.go — Fix 1 (null field rejection)
• Read manifest.go and manifest_test.go — Fix 2 (exclusion flag presence)
• Diff against base branch to isolate round 7 changes
• Check carry-over items from previous rounds
• Post full review

Both stated fixes are correct and well-tested. Four new issue references added this round; one maintenance concern on exclusionsRequiredFields; the eight accumulated comment items from prior rounds are still open.

Fix 1 — Null KEYMAP field rejection via jsonNullLiteral ✅

keymap.go:15-18 — var jsonNullLiteral = []byte("null") as a package-level sentinel is the right tool for raw JSON comparison. The check placed inside the [...]string{"encoded","original","kind"} loop at keymap.go:193-202 correctly handles all three fields symmetrically before the typed unmarshal. encoded and kind are already guarded by the empty-string check that follows, but applying the null guard uniformly prevents any future-field addition from silently inheriting the null-collapse problem. ✓

TestKeymapReader_RejectsExplicitNullField (keymap_test.go:227-247) tests all three sub-cases (null original, null encoded, null kind). ✓

Observation — //nolint:mnd in keymap_test.go:286 still present (flagged round 6, not yet fixed):

original[i] = byte(i % 251) //nolint:mnd // arbitrary byte spread

CLAUDE.md: "Avoid //nolint — refactor instead." Named constant resolves this:

const testBytePrimeSpread = 251
// ...
original[i] = byte(i % testBytePrimeSpread)

Fix 2 — validateExclusionsFieldsPresent ✅ (with one maintenance note)

manifest.go:288-315 — The two-phase approach (struct decode for type checking, raw JSON walk for presence checking) is exactly what's needed when boolean fields silently default to false on absence. The early-return at manifest.go:294-297 (when exclusions is absent from the top-level JSON) is correct — the nil-pointer check in validateRequiredFields already surfaces that case with a cleaner error. ✓

TestReadManifest_RejectsMissingExclusionFlag (manifest_test.go:389-443) covers all 4 missing-flag cases plus the explicit-null case. ✓

Maintenance concern — exclusionsRequiredFields must stay in sync with Exclusions struct:

manifest.go:321-326 — The array is manually maintained alongside the Exclusions struct. The comment warns about it but there is no compile-time enforcement. If a 5th field (say include_delete_markers bool) is added to Exclusions, the new flag will silently pass as false on absence until someone remembers to add it to exclusionsRequiredFields. The current test covers exactly the current 4 fields — a new field wouldn't be caught by the existing table-driven test either. Not a blocker, but worth noting in the PR description so the next contributor knows where to look.

New issue references added this round (4) ⚠️

keymap.go:187 — Codex P2 round 5 + P1 round 7-follow-up. → drop both references.
keymap.go:191 — Codex P1 #179. → drop (was also flagged in rounds 5 and 6).
manifest.go:285 — (Codex P2 round 7) → drop.
manifest_test.go:390 — Codex P2 round 7: → change is the regression for Codex P2 round 7: → verifies that.

Accumulated carry-over items — still open after 7 rounds

Corrected text for manifest.go:260-266 (unchanged since round 1):

// MANIFEST.json is exactly one JSON object. Any non-whitespace
// decodable content after it (a second object, junk) points at
// concatenation bugs or partial-write corruption. Trailing
// whitespace and newlines are benign and not flagged.

Fix all 13 accumulated comment items in one pass →

Summary

The Round 7 fixes in 6b2dea00 are both correct — null-field detection in decodeKeymapLine and presence validation in validateExclusionsFieldsPresent are sound approaches, and all new tests cover the stated failure modes. The only logic-level note is the exclusionsRequiredFields maintenance hazard (worth a PR description callout). Everything else remaining is comment cleanup: 4 new issue references this round plus 9 accumulated from prior rounds that have now been flagged for at least 2 rounds each.

@claude review

Round 8 fixes for Codex P2 round 8 on commit 6b2dea0 (commit f13cd1bf):

1. Missing format_version misclassified as upgrade-required. The probe decoded directly into uint32, so an absent field collapsed silently to 0 and the gate returned ErrUnsupportedFormatVersion ("format_version is zero"). A truncated/malformed manifest belongs in ErrInvalidManifest so callers branching on errors.Is can distinguish "this file is broken" from "newer producer". Now a presence-and-null probe runs on the raw JSON format_version token before the typed unmarshal; the whole probe is extracted into probeManifestFormatVersion to keep ReadManifest's cyclomatic complexity under the linter ceiling. Tests: TestReadManifest_RejectsMissingFormatVersion (also asserts the error is not ErrUnsupportedFormatVersion) and TestReadManifest_RejectsNullFormatVersion.

2. WriteManifest accepted future format_version. A caller mutating m.FormatVersion = CurrentFormatVersion + 1 could write a manifest that the same package's ReadManifest then refuses — self-incompatible backup metadata for a one-line mutation. validateRequiredFields now rejects that case with ErrInvalidManifest. Test: TestWriteManifest_RejectsFutureFormatVersion.

go test -race and golangci-lint clean. Fix merged forward into #713 / #714 / #716 / #718.

Claude finished @bootjp's task in 2m 48s —— View job

Review of Round 8 fixes (f13cd1bf) — format_version probing + write-side future version guard

Tasks:

• Read trigger comment and understand scope
• Read manifest.go and manifest_test.go — both fixes
• Diff against base branch to isolate round 8 changes
• Review probeManifestFormatVersion and missing/null format_version handling
• Review WriteManifest future format_version guard
• Check carry-over items from previous rounds
• Post full review

Both stated fixes are correct and well-tested. Four new issue references added this round; the accumulated carry-over list now spans 18 items across 4 files.

Fix 1 — probeManifestFormatVersion extraction + missing/null handling ✅

manifest.go:281-307 — The three-tier gate is now correct and complete:

missing / null  → ErrInvalidManifest   (malformed/truncated)
zero            → ErrUnsupportedFormatVersion  (reserved sentinel)
> current       → ErrUnsupportedFormatVersion  (upgrade required)
within range    → nil

The map[string]json.RawMessage probe before the typed unmarshal is the right approach — json.Unmarshal into uint32 silently collapses absence to 0, which was the root of the misclassification. The bytes.Equal(rawFV, jsonNullLiteral) guard is correctly reusing the package-level sentinel from keymap.go. Extracting to probeManifestFormatVersion keeps ReadManifest clean.

TestReadManifest_RejectsMissingFormatVersion (manifest_test.go:179-201) — tests both halves of the fix: ErrInvalidManifest is returned AND ErrUnsupportedFormatVersion is not. ✓

TestReadManifest_RejectsNullFormatVersion (manifest_test.go:205-225) — mirrors the missing case for explicit null. ✓

Fix 2 — Future version guard in validateRequiredFields ✅

manifest.go:377-380 — The write-path guard correctly returns ErrInvalidManifest (not ErrUnsupportedFormatVersion). The intentional asymmetry is sound:

• Read path (probeManifestFormatVersion): ErrUnsupportedFormatVersion — tells callers "upgrade required, this file is well-formed but newer than me"
• Write path (validateRequiredFields): ErrInvalidManifest — tells producers "your manifest struct is internally inconsistent, this build cannot produce that version"

TestWriteManifest_RejectsFutureFormatVersion (manifest_test.go:233-242) — confirms the producer-side gate. ✓

Note on TestReadManifest_RejectsFutureFormatVersion at manifest_test.go:121-131: this test bypasses WriteManifest via json.Marshal to construct an unwriteable manifest — that's the correct approach for testing the read path in isolation. The round-8 future-version check in validateRequiredFields adds a second catch point on the read path (inside m.validate()), but probeManifestFormatVersion runs first and already returns ErrUnsupportedFormatVersion. The double-check is harmless dead code on the read path.

New issue references added this round (4) ⚠️

manifest.go:275 — Codex P2 round 8 in probeManifestFormatVersion docstring:

//   - missing or null `format_version` -> ErrInvalidManifest
//     (truncated/malformed file; Codex P2 round 8). Without this

Drop Codex P2 round 8.

manifest.go:376 — Codex P2 round 8. in validateRequiredFields:

// backup metadata. Codex P2 round 8.

Drop Codex P2 round 8..

manifest_test.go:174 — Codex P2 round 8: in test comment:

// TestReadManifest_RejectsMissingFormatVersion is the regression for
// Codex P2 round 8: an absent `format_version` unmarshals...

Change to TestReadManifest_RejectsMissingFormatVersion verifies that....

manifest_test.go:228 — Codex P2 round 8: in test comment:

// TestWriteManifest_RejectsFutureFormatVersion is the regression for
// Codex P2 round 8: WriteManifest must refuse manifests advertising

Change to TestWriteManifest_RejectsFutureFormatVersion verifies that WriteManifest refuses....

Accumulated carry-over items — 18 items across 4 files

The stale trailing-bytes comment (manifest.go:247-253) has been flagged every round since round 1. Corrected text (unchanged from prior rounds):

// MANIFEST.json is exactly one JSON object. Any non-whitespace
// decodable content after it (a second object, junk) points at
// concatenation bugs or partial-write corruption. Trailing
// whitespace and newlines are benign and not flagged.

Fix all 22 accumulated comment issues in one pass →

Summary

The Round 8 fixes in f13cd1bf are both correct. probeManifestFormatVersion properly distinguishes the three distinct failure modes (malformed, reserved-zero, upgrade-required) and the write-side future-version guard closes the self-incompatibility gap. All four new tests are well-targeted. The only remaining work is comment cleanup — 4 new issue references this round plus 14 accumulated from prior rounds, plus the stale io.Discard/whitespace comment that has been open since round 1. A single pass across all 4 files would clear the board.